Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix full modal nonlinear polarisation #247

Merged
merged 14 commits into from
Oct 25, 2021

Conversation

chrisbrahms
Copy link
Collaborator

This should (I think) fix #246

dimlimits for MarcatiliModes are now (0, π) rather than (0, 2π). There is a new function Modes.geomfac which takes this into account by multiplying the integral in TransModal by 2. For other modes this defaults to 1. With this change, the initial points at which pcubature evaluates the integrand are now (0, π/2, π), so it no longer gets stuck.

I still have to fully convince myself that this is general--so far it produces correct results for both LP11 propagation (new test in test_polarisation_field.jl and HE11 propagation (test_multimode.jl).

As it stands this will mess up derived modes, like for example a ZeisbergerMode, because they don't implement geomfac. So the brute-force method of calculating Modes.N and Modes.Aeff no longer produces correct results.

TODO:

  • More comprehensive tests
  • Move multiplication by geomfac into the loop in TransModal to avoid the extra array multiplication
  • Think about a better way of working out the multiplication factor to keep dimlimits the same

@chrisbrahms chrisbrahms requested a review from jtravs October 13, 2021 14:20
@chrisbrahms
Copy link
Collaborator Author

Ok this definitely does not work. I get the wrong answer when pumping in LP11 but with HE11 present. With Kerr only, there should be no nonlinear polarisation in HE11, but I get the same as in HE21 and TM01:
image
(the modes here are TM01, HE21, HE11)

This is because HE11 and HE21/TM01 don't have the same symmetry, so we do actually need to integrate (0, 2π) for the integral to vanish.

I think our best option for now is actually to switch to hcubature. A quick test with just two modes (HE21 and TM01 as I've been using) shows it's actually faster. With this PR:

julia> @btime transform_vert(nl_vert, Eω_vert, 0.0);
  266.604 ms (8471 allocations: 99.19 MiB)
julia> transform_vert.ncalls
527

Without this PR, but switching to hcubature_v in TransModal for full 2D integrals:

julia> @btime transform_vert(nl_vert, Eω_vert, 0.0);
  155.324 ms (5195 allocations: 60.84 MiB)
julia> transform_vert.ncalls
323

I think it's possible that when @jtravs measured the performance of pcubature_v vs hcubature_v before, pcubature_v was coming out faster simply because it was ignoring most of the integration domain.

@jtravs
Copy link
Contributor

jtravs commented Oct 15, 2021

I'm much happier with this, and I think you're correct about my benchmarks. I will review once I see the tests have passed.

@jtravs
Copy link
Contributor

jtravs commented Oct 15, 2021

Wait, why are tests skipped?

@chrisbrahms chrisbrahms marked this pull request as ready for review October 15, 2021 10:05
@chrisbrahms
Copy link
Collaborator Author

Tests were skipped because I marked this as draft. They're running now.

@chrisbrahms
Copy link
Collaborator Author

There is still a stack overflow error on the v1.5 tests, but I'm pretty certain it's not this PR (see #248 ) so I'll merge.

I think the error is due to one of our dependencies getting a minor update. Need to check in more detail.

@chrisbrahms chrisbrahms merged commit 8858ef5 into LupoLab:master Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full modal integral does not give correct results
2 participants